Clone region var origins instead of taking them in borrowck #109753

compiler-errors · 2023-03-30T03:20:05Z

Fixes an issue with the new solver where reporting a borrow-checker error ICEs because it calls InferCtxt::evaluate_obligation.

This also removes a handful of unnecessary tcx.infer_ctxt().build() calls that are only there to mitigate this same exact issue, but with the old solver.

Fixes compiler-errors/next-solver-hir-issues#12.

This implements @aliemjay's solution where we just don't take the region constraints, but clone them. This potentially makes it easier to write a bug about taking region constraints twice or never at all, but again, not many folks are touching this code.

rustbot · 2023-03-30T03:20:11Z

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

compiler-errors · 2023-03-30T03:20:34Z

Grr, meant to r? @ghost for now.

lcnr

nit

r=me afterwards

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

lcnr · 2023-03-30T09:46:33Z

compiler/rustc_infer/src/infer/mod.rs

-            .as_mut()
-            .expect("region constraints already solved")
+            // Replace region constraints if they've been taken.
+            // This should only happen on an error path, so delay a bug.


Suggested change

// This should only happen on an error path, so delay a bug.

// This should only happen on an error path, so delay a bug.

//

// This is only allowed so one can use the existing inference

// context for diagnostics.

aliemjay · 2023-03-30T13:38:00Z

compiler/rustc_infer/src/infer/mod.rs

+            .get_or_insert_with(|| {
+                ty::tls::with(|tcx| {
+                    tcx.sess.delay_span_bug(DUMMY_SP, "region constraints already solved");
+                });
+                RegionConstraintStorage::default()
+            })


I don't think RegionConstraintStorage::default() is a sane value. This resets the region variables counter and thus nullifies the whole point of using the existing InferCtxt.

How about changing InferCtxt::take_region_var_origins to clone region VarInfos instead of removing them? and probably using better name for it.

Maybe we can keep it named take_ name, but set some flag that the infos have been taken, and delay a bug if we try to take them again or try to borrow them again?

Mm, I have no strong opinion here, but I don't like delaying a bug in InferCtxt. I prefer the code there to be independent from the compilation session as much as possible.

Changing take_region_var_infos is unfortunate as it would no longer guard against future addition of region constraints, but I think it's the best option here given that it's used only once in borrowck and we don't expect users of InferCtxt to implement their own region resolution.

bors · 2023-04-03T01:09:52Z

☔ The latest upstream changes (presumably #109849) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2023-04-16T19:33:06Z

Ok, took the alternative approach.

@rustbot ready

aliemjay · 2023-04-16T23:54:03Z

compiler/rustc_infer/src/infer/mod.rs

-    /// `resolve_regions_and_report_errors` is invoked, this gets set to `None`
-    /// -- further attempts to perform unification, etc., may fail if new
-    /// region constraints would've been added.
-    region_constraint_storage: Option<RegionConstraintStorage<'tcx>>,


I think it's worth keeping this Option and clearing it in InferCtxt::{resolve_regions,skip_region_resolution} . This way limit this hack to MIR borrowck, rather that lexical region resolver which is more commonly used.

aliemjay

r? @aliemjay
looks good. r=me with or without the previous nit.

compiler-errors · 2023-04-17T17:57:32Z

Ok, changed it back to Option.

@bors r=aliemjay

bors · 2023-04-17T17:57:34Z

📌 Commit 72ffa1160aab7b92a20260c3bbe651119935952e has been approved by aliemjay

It is now in the queue for this repository.

bors · 2023-04-17T19:46:25Z

☔ The latest upstream changes (presumably #110458) made this pull request unmergeable. Please resolve the merge conflicts.

compiler-errors · 2023-04-21T04:14:23Z

Rebased

@bors r=aliemjay

bors · 2023-04-21T04:14:25Z

📌 Commit 964600b has been approved by aliemjay

It is now in the queue for this repository.

bors · 2023-04-22T15:15:54Z

⌛ Testing commit 964600b with merge 4396cec...

bors · 2023-04-22T17:20:22Z

☀️ Test successful - checks-actions
Approved by: aliemjay
Pushing 4396cec to master...

rust-timer · 2023-04-22T19:26:39Z

Finished benchmarking commit (4396cec): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.5%	[0.5%, 0.5%]	1

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.2%	[1.2%, 1.2%]	1
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-2.4%	[-2.8%, -2.0%]	4
All ❌✅ (primary)	-2.5%	[-2.5%, -2.5%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

…r=compiler-errors Fix comment for `get_region_var_origins` rust-lang#109753 changed the logic but not the comment. r? `@compiler-errors`

rustbot assigned petrochenkov Mar 30, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 30, 2023

compiler-errors unassigned petrochenkov Mar 30, 2023

lcnr reviewed Mar 30, 2023

View reviewed changes

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs Outdated Show resolved Hide resolved

lcnr reviewed Mar 30, 2023

View reviewed changes

aliemjay requested changes Mar 30, 2023

View reviewed changes

rust-cloud-vms bot force-pushed the replenish-region-constraints branch from 4e1c9b3 to 08101d6 Compare April 16, 2023 19:32

compiler-errors marked this pull request as ready for review April 16, 2023 19:33

rust-cloud-vms bot force-pushed the replenish-region-constraints branch from 08101d6 to 3d604ea Compare April 16, 2023 19:44

aliemjay reviewed Apr 16, 2023

View reviewed changes

aliemjay approved these changes Apr 16, 2023

View reviewed changes

rustbot assigned aliemjay Apr 16, 2023

rust-cloud-vms bot force-pushed the replenish-region-constraints branch from 3d604ea to 72ffa11 Compare April 17, 2023 16:44

compiler-errors changed the title ~~Replace region constraints if InferCtxt::probe is called after resolve_regions~~ Clone region var origins instead of taking them in borrowck Apr 17, 2023

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2023

matthiaskrgr mentioned this pull request Apr 17, 2023

Rollup of 6 pull requests #110463

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 17, 2023

Clone region var origins instead of taking in borrowck

964600b

rust-cloud-vms bot force-pushed the replenish-region-constraints branch from 72ffa11 to 964600b Compare April 21, 2023 00:41

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 22, 2023

bors merged commit 4396cec into rust-lang:master Apr 22, 2023

rustbot added this to the 1.71.0 milestone Apr 22, 2023

bors mentioned this pull request Apr 22, 2023

Move around {Idx, IndexVec, IndexSlice} adjacent code #110539

Merged

Noratrieb mentioned this pull request Jun 6, 2023

Fix comment for get_region_var_origins #112356

Merged

compiler-errors deleted the replenish-region-constraints branch August 11, 2023 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clone region var origins instead of taking them in borrowck #109753

Clone region var origins instead of taking them in borrowck #109753

compiler-errors commented Mar 30, 2023 •

edited

Loading

rustbot commented Mar 30, 2023

compiler-errors commented Mar 30, 2023

lcnr left a comment

lcnr Mar 30, 2023 •

edited

Loading

aliemjay Mar 30, 2023

compiler-errors Apr 2, 2023

aliemjay Apr 3, 2023

bors commented Apr 3, 2023

compiler-errors commented Apr 16, 2023

aliemjay Apr 16, 2023

aliemjay left a comment •

edited

Loading

compiler-errors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

compiler-errors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 22, 2023

bors commented Apr 22, 2023

rust-timer commented Apr 22, 2023

Clone region var origins instead of taking them in borrowck #109753

Clone region var origins instead of taking them in borrowck #109753

Conversation

compiler-errors commented Mar 30, 2023 • edited Loading

rustbot commented Mar 30, 2023

compiler-errors commented Mar 30, 2023

lcnr left a comment

Choose a reason for hiding this comment

lcnr Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

aliemjay Mar 30, 2023

Choose a reason for hiding this comment

compiler-errors Apr 2, 2023

Choose a reason for hiding this comment

aliemjay Apr 3, 2023

Choose a reason for hiding this comment

bors commented Apr 3, 2023

compiler-errors commented Apr 16, 2023

aliemjay Apr 16, 2023

Choose a reason for hiding this comment

aliemjay left a comment • edited Loading

Choose a reason for hiding this comment

compiler-errors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

compiler-errors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 22, 2023

bors commented Apr 22, 2023

rust-timer commented Apr 22, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

compiler-errors commented Mar 30, 2023 •

edited

Loading

lcnr Mar 30, 2023 •

edited

Loading

aliemjay left a comment •

edited

Loading